ci: fix skipped tests #155

muzarski · 2024-08-05T09:11:59Z

This PR addresses multiple issues:

Skipped tests for scylla clusters

As linked issue mentions, ccm <node> version for scylla clusters always returns 3.0.8 version which I believe is hardcoded. Because of that, some tests were unnecessarily skipped. To retrieve scylla version, we should use ccm <node> versionfrombuild.

I adjusted the logic for retrieving the Scylla version. Apart from that, I skip the version checks for Scylla, since the feature to version mapping is not the same for Scylla and Cassandra. I left the checks for Cassandra. As of now, I don't think it's really important to check which features should be tested based on Scylla version, since we don't plan to test old Scylla versions. We will add such filters in the future, once they are needed.

No compact test

Previously, this test was silently passing as it was skipped for both Scylla and Cassandra. However, the function cass_cluster_set_no_compact is not even implemented. I removed this test from CI (for both Scylla and Cassandra).

Map collection iterator

cpp-rust-driver's implementation of cass_iterator_from_collection for maps was inconsistent with cpp-driver's implementation. This PR fixes it. I found this bug, because the test for a map<int, duration> type was handled a bit differently than for the other types (since apparently, duration cannot be used as a primary key). This test was failing due to the bug with map iterators.

Collection objects in test framework

The Collection object was not initialized correctly when constructed from CassValue. An issue is described in detail in a corresponding commit.

Modified tests for collections

Due to the two previous bugs, the tests for collections were modified a bit in the past. Since the issues are fixed, I reverted these changes and the tests now contain original logic from the upstream.

Duration tests for cassandra

Enabled duration tests for cassandra.

Scylla version in CI

Bumped Scylla version in CI to 6.0.0.

Pre-review checklist

I have split my patch into logically separate commits.
All commit messages clearly explain what they change and why.
PR description sums up the changes and reasons why they should be introduced.
I have enabled appropriate tests in .github/workflows/build.yml in gtest_filter.
I have enabled appropriate tests in .github/workflows/cassandra.yml in gtest_filter.

Support for duration type was missing for some reason. Fixing it.

muzarski · 2024-08-05T15:57:09Z

Rebased on master

tests/src/integration/ccm/bridge.cpp

Lorak-mmk · 2024-08-05T18:01:09Z

tests/src/integration/tests/test_basics.cpp

@@ -343,7 +343,7 @@ CASSANDRA_INTEGRATION_TEST_F(BasicsTests, NoCompactEnabledConnection) {
    }


Aren't we going to run into this if (!Options::is_cassandra()) { if (server_version_ >= "6.0.0") { branch when we update Scylla to 6.0?
If so, can you fix this in this PR?

No, Options::is_cassandra() actually returns true for Scylla as well. It's because Scylla identifies as Apache Cassandra server (such info is retrieved from ccm).

There are 3 possible CCM:ServerType enum values:

bool Options::is_cassandra() { return server_type_ == CCM::ServerType::CASSANDRA; } bool Options::is_dse() { return server_type_ == CCM::ServerType::DSE; } bool Options::is_ddac() { return server_type_ == CCM::ServerType::DDAC; }

I'm not sure about the difference between the types, but in Scylla's case, the server type is CASSANDRA. Options::is_scylla_ is a flag responsible for distinguishing between scylla and cassandra.

wth is ddac?! Never heard about it before.

https://www.datastax.com/press-release/datastax-announces-datastax-distribution-apache-cassandratm

muzarski · 2024-08-07T07:50:37Z

v1.1: addressed review comment

tests/src/integration/ccm/bridge.cpp

wprzytula · 2024-08-12T07:23:25Z

tests/src/integration/ccm/cass_version.hpp

    std::string scylla_version_prefix = "release:";
    std::string version(version_string);
    if (version.compare(0, scylla_version_prefix.size(), scylla_version_prefix) == 0) {
-      version = "3.0.8";
+      version.replace(0, scylla_version_prefix.size(), "");
    }
    std::replace(version.begin(), version.end(), '.', ' ');


How this new code actually retrieves the Scylla version? Or, if it's already retrieved, what variable stores it?

Or, if it's already retrieved, what variable stores it?

The version variable.

tests/src/integration/integration.hpp

.github/workflows/build.yml

wprzytula · 2024-08-12T08:04:29Z

tests/src/integration/objects/collection.hpp

      // Initialize the iterator
      iterator_ = cass_iterator_from_collection(value);

-      // Determine if the collection is empty (null)
-      const CassValue* check_value = cass_iterator_get_value(iterator_.get());
-      if (check_value) {
-        is_null_ = false;
-      }
+      // We already checked that collection is not null.
+      is_null_ = false;


While this is an unpleasant peculiarity, empty collections seem to be identical to NULL values for the DB. See scylladb/scylla-rust-driver#1007 for reference.

Have you considered making position field a non-Option?

I don't want to dive too deep into the code (because I have other tasks to do), but I think we should do our best to make the cpp-rust-driver's behaviour consistent with what the official cpp-driver's tests expect.

I don't want to dive too deep into the code (because I have other tasks to do), but I think we should do our best to make the cpp-rust-driver's behaviour consistent with what the official cpp-driver's tests expect.

It was my understanding that this is what this PR does. Is that not the case?

While this is an unpleasant peculiarity, empty collections seem to be identical to NULL values for the DB. See scylladb/scylla-rust-driver#1007 for reference.

Good catch. I'll add the is_null_ initialization here. However, I still think that iterator should not be used for checking if collection is empty. The iterator, when created, is UNINITIALIZED - to retrieve the first value, we should firstly advance the iterator via cass_iterator_next. I don't want to advance the iterator just to check if it's empty. I'll adjust the code so we make use of cass_value_item_count function, which can be used to see if collection is empty.

muzarski · 2024-09-04T13:59:28Z

v2: addressed review comments

tests/src/integration/ccm/bridge.cpp

tests/src/integration/integration.hpp

dkropachev · 2024-09-05T05:47:21Z

tests/src/integration/objects/collection.hpp

+      size_t collection_size = cass_value_item_count(value);
+      if (collection_size > 0) {
+        is_null_ = false;
+      }

      // Initialize the iterator
      iterator_ = cass_iterator_from_collection(value);


Can you drop this line then ?

No. iterator_ is a field. This code is called when constructor is called, and so this line initializes a field of a new object.

For some reason `ccm <node> version`` for scylla cluster always returns 3.0.8. To return a correct scylla version, we need to make use of `ccm <node> versionfrombuild`.

Retrieve a scylla version from CLI, instead of hardcoding it to 3.0.8

Some Scylla tests were unnecesarily skipped because of minimal required version for cassandra. Let's skip the check for Scylla clusters. We will add those check in the future if it's necessary to do so. As of now, they are not needed.

cass_cluster_set_no_compact is not implemented, so the corresponding test should be disabled. Now, it is "passing", since it is being skipped due to cassandra version filtering. It won't be true after the next commit.

Previously, we would return a MapIterator from cass_iterator_from_collection in case map value was passed. This approach is not consistent with cpp-driver's logic. We should return: - CollectionIterator from cass_iterator_from_collection (for lists, sets, maps and tuples) - MapIterator only if user called cass_iterator_from_map directly

`cass_iterator_get_value` function should not be used to determine whether the collection is empty... It was working for cpp-driver, since this function always returns a non-null pointer there. In case of cpp-rust-driver, we return a null pointer when user calls `cass_iterator_get_value` before `cass_iterator_next`, because the latter function is responsible for advancing the iterator, whose state is uninitialized upon creation (position is None). OTOH, cpp-driver returns a pointer to uninitialized value when calling `cass_iterator_get_value` first - this is why `is_null_` was always false for cpp-driver. Now, to check if collection is empty, we make use of `cass_value_item_count` API function.

Two previous commits fixed the issues that forced us to modify the logic for collection types tests. Since the issues are fixed, we can revert these changes.

muzarski · 2024-09-05T07:37:59Z

v2.1: Addressed @dkropachev 's review comments.

muzarski marked this pull request as draft August 5, 2024 09:12

muzarski force-pushed the fix_skipped_tests branch 6 times, most recently from fdb5183 to be3ade0 Compare August 5, 2024 15:00

muzarski marked this pull request as ready for review August 5, 2024 15:33

muzarski self-assigned this Aug 5, 2024

muzarski requested review from Lorak-mmk and wprzytula August 5, 2024 15:34

types: convert column type duration to cass data type

0759fe1

Support for duration type was missing for some reason. Fixing it.

muzarski force-pushed the fix_skipped_tests branch from be3ade0 to b5b28fc Compare August 5, 2024 15:57

Lorak-mmk requested changes Aug 5, 2024

View reviewed changes

muzarski force-pushed the fix_skipped_tests branch from b5b28fc to a954104 Compare August 7, 2024 07:49

muzarski requested a review from Lorak-mmk August 7, 2024 07:50

Lorak-mmk approved these changes Aug 8, 2024

View reviewed changes

wprzytula requested changes Aug 12, 2024

View reviewed changes

muzarski force-pushed the fix_skipped_tests branch from a954104 to 414817c Compare September 4, 2024 13:58

muzarski force-pushed the fix_skipped_tests branch from 414817c to 303db16 Compare September 4, 2024 14:03

muzarski requested a review from dkropachev September 4, 2024 14:08

dkropachev requested changes Sep 5, 2024

View reviewed changes

muzarski added 5 commits September 5, 2024 09:31

ccm: get version from scylla

6e0910c

For some reason `ccm <node> version`` for scylla cluster always returns 3.0.8. To return a correct scylla version, we need to make use of `ccm <node> versionfrombuild`.

cass_version: don't hardcode scylla version

8e9f164

Retrieve a scylla version from CLI, instead of hardcoding it to 3.0.8

test: check version only for cassandra

a1df254

Some Scylla tests were unnecesarily skipped because of minimal required version for cassandra. Let's skip the check for Scylla clusters. We will add those check in the future if it's necessary to do so. As of now, they are not needed.

ci: disable no_compact test

82804ad

cass_cluster_set_no_compact is not implemented, so the corresponding test should be disabled. Now, it is "passing", since it is being skipped due to cassandra version filtering. It won't be true after the next commit.

test: skip version check for scylla in nocompact test

1ccd2bc

muzarski added 5 commits September 5, 2024 09:37

test: revert changes from collection types test

aa7312f

Two previous commits fixed the issues that forced us to modify the logic for collection types tests. Since the issues are fixed, we can revert these changes.

ci: enable duration tests for cassandra

8a15682

ci: bump scylla version to 6.0.0

d5cff5c

muzarski force-pushed the fix_skipped_tests branch from 303db16 to d5cff5c Compare September 5, 2024 07:37

muzarski requested a review from dkropachev September 5, 2024 07:41

dkropachev merged commit 7fc403b into scylladb:master Sep 5, 2024
5 checks passed

muzarski mentioned this pull request Oct 3, 2024

bump version to 0.2.0 #184

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: fix skipped tests #155

ci: fix skipped tests #155

muzarski commented Aug 5, 2024 •

edited

Loading

muzarski commented Aug 5, 2024

Lorak-mmk Aug 5, 2024

muzarski Aug 5, 2024 •

edited

Loading

Lorak-mmk Aug 5, 2024

wprzytula Aug 8, 2024

muzarski commented Aug 7, 2024

wprzytula Aug 12, 2024 •

edited by muzarski

Loading

muzarski Sep 4, 2024

wprzytula Aug 12, 2024

wprzytula Aug 12, 2024

wprzytula Aug 12, 2024

Lorak-mmk Aug 12, 2024

muzarski Sep 4, 2024 •

edited

Loading

muzarski commented Sep 4, 2024

dkropachev Sep 5, 2024

muzarski Sep 5, 2024

muzarski commented Sep 5, 2024

		@@ -343,7 +343,7 @@ CASSANDRA_INTEGRATION_TEST_F(BasicsTests, NoCompactEnabledConnection) {
		}

ci: fix skipped tests #155

ci: fix skipped tests #155

Conversation

muzarski commented Aug 5, 2024 • edited Loading

Skipped tests for scylla clusters

No compact test

Map collection iterator

Collection objects in test framework

Modified tests for collections

Duration tests for cassandra

Scylla version in CI

Pre-review checklist

muzarski commented Aug 5, 2024

Choose a reason for hiding this comment

muzarski Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muzarski commented Aug 7, 2024

wprzytula Aug 12, 2024 • edited by muzarski Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muzarski Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

muzarski commented Sep 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muzarski commented Sep 5, 2024

muzarski commented Aug 5, 2024 •

edited

Loading

muzarski Aug 5, 2024 •

edited

Loading

wprzytula Aug 12, 2024 •

edited by muzarski

Loading

muzarski Sep 4, 2024 •

edited

Loading